-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Ingest Manager] Remove default namespace toggle #66298
[Ingest Manager] Remove default namespace toggle #66298
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
id="xpack.ingestManager.agentConfigForm.namespaceUseDefaultsFieldLabel" | ||
defaultMessage="Use default namespace" | ||
id="xpack.ingestManager.agentConfigForm.namespaceFieldHelpText" | ||
defaultMessage="Leave empty if you do not want a namespace." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. There is always a namespace called default
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruflin That's new to me, I thought we allowed no namespaces. So if the user leaves this input empty, we add default
to the agent config behind the scenes? Is that the same for the namespace field when creating a data source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There must always be a namespace and agent will fill it in if it is not provided. @ph Just to confirm.
I'm still on the fence if we need this setting here at all on the config level. If we do, we must probably better describe what it is for, meaning telling the user this is about where the data is shipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be something to look at on the agent (and/or EPM?) side. I've used agent configs with no namespaces that end up creating indices without a namespace suffix like metrics-system.core-
.
I'll confirm this behavior and create follow up issues where we can discuss how to handle empty namespaces. I think that would be separate from this PR as the PR is UI-only change. The "Leave empty if you do not want a namespace" copy is correct with regards to how we currently handle things, even though it's not the behavior we want.
edef68e
to
560bc83
Compare
I've updated this PR to enforce non-empty value for namespace. @elastic/ingest-management Could someone review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this works as expected.
The UI does not allow me to create a config without a namespace, that's 👍
When I use a new namespace test
though, it is not shown in the configuration details (though it does appear in the API response):
For comparison, the pre-existing default config shows the default
namespace:
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jen-huang Thanks for creating an issue!
* Remove default namespace toggle * Fix i18n * Add namespace validation Co-authored-by: Elastic Machine <[email protected]>
* Remove default namespace toggle * Fix i18n * Add namespace validation Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Resolves #65666. Removes
Use default namespace
toggle to match designs and adds validation when namespace field is empty.